Call into PerfMap logic exclusively in preemptive mode#129019
Conversation
…ere Contracts are supported)
…e various techniques for getting an entrypoint, and FindAssociatedMethodDesc are called in preemptive code. This has the effect that the PerfMap::LogStubs logic is always called in preemptive mode, and thus taking the critical section in it won't trigger a GC transition.
|
Tagging subscribers to this area: @agocke |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR VM call paths and contracts so PerfMap-related entrypoints (and a set of closely-related MethodDesc/slot resolution APIs) are only invoked in preemptive GC mode, and it removes many FEATURE_PERFMAP include/usage guards by providing always-available PerfMap APIs (with stubs when the feature is off).
Changes:
- Introduces/uses conditional
GCX_PREEMP/GCX_MAYBE_PREEMPtransitions around stub generation and MethodDesc entrypoint resolution to satisfy PerfMap and related contract requirements. - Removes
#ifdef FEATURE_PERFMAPwrappers at many call sites and centralizes “no-feature” behavior via stubbed PerfMap APIs. - Adjusts multiple VM contracts (e.g.,
MODE_ANY→MODE_PREEMPTIVE) and refactors some call helpers / delegate QCall plumbing accordingly.
Reviewed changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/vm/virtualcallstub.cpp | Wraps stub generation with conditional preemptive transitions; unguards PerfMap logging. |
| src/coreclr/vm/threads.cpp | Unguards PerfMap logging for write barrier copy stubs. |
| src/coreclr/vm/stublink.cpp | Unguards PerfMap logging for emitted stubs. |
| src/coreclr/vm/runtimehandles.cpp | Narrows cooperative transitions in QCalls; adds preemptive transition for wrapper-stub unwrap. |
| src/coreclr/vm/riscv64/stubs.cpp | Unguards dynamic helper PerfMap logging macro. |
| src/coreclr/vm/reflectioninvocation.cpp | Moves call-target retrieval into a preemptive region. |
| src/coreclr/vm/readytoruninfo.cpp | Unguards PerfMap logging for dynamic helper precodes. |
| src/coreclr/vm/qcall.h | Adds ObjectHandleOnStack::IsNull() helper. |
| src/coreclr/vm/prestub.cpp | Unguards PerfMap logging; adds preemptive transition in dynamic helper worker path. |
| src/coreclr/vm/precode.cpp | Unguards PerfMap logging for various precode allocations. |
| src/coreclr/vm/perfmap.h | Adds stub PerfMap class under !FEATURE_PERFMAP; keeps real implementation under feature. |
| src/coreclr/vm/methodtable.inl | Changes GetMethodDescForSlot contract to MODE_PREEMPTIVE. |
| src/coreclr/vm/methodtable.cpp | Shifts several contracts to MODE_PREEMPTIVE; adds local coop regions for managed ref access. |
| src/coreclr/vm/method.hpp | Removes declaration of FindOrCreateExactClassMethod. |
| src/coreclr/vm/method.cpp | Broad contract/mode changes and added coop subregions for OBJECTREF access. |
| src/coreclr/vm/loongarch64/stubs.cpp | Unguards dynamic helper PerfMap logging macro. |
| src/coreclr/vm/jitinterface.cpp | Unguards PerfMap header include. |
| src/coreclr/vm/jithelpers.cpp | Adds preemptive transition before virtualized call target resolution. |
| src/coreclr/vm/interpexec.cpp | Adds preemptive transitions around virtualized MethodDesc resolution in interpreter. |
| src/coreclr/vm/i386/cgenx86.cpp | Unguards dynamic helper PerfMap logging macro. |
| src/coreclr/vm/genmeth.cpp | Removes helper API; adjusts contracts for instantiation/associated MethodDesc creation. |
| src/coreclr/vm/fptrstubs.cpp | Adds preemptive transition earlier in FuncPtr stub creation path. |
| src/coreclr/vm/eventing/eventpipe/ds-rt-coreclr.h | Updates contracts around perfmap enable path. |
| src/coreclr/vm/dllimportcallback.cpp | Unguards PerfMap logging and updates mode contracts for UM entry thunk paths. |
| src/coreclr/vm/dispatchinfo.cpp | Reorders GC mode transitions; shifts GetMemberInfoMap contract to preemptive. |
| src/coreclr/vm/customattribute.cpp | Precomputes ctor call target in preemptive mode and uses new callsite ctor. |
| src/coreclr/vm/corhost.cpp | Wraps GetSingleCallableAddrOfCode in preemptive region. |
| src/coreclr/vm/comutilnative.cpp | Avoids holding OBJECTREF across regions; uses MethodTable* in preemptive mode. |
| src/coreclr/vm/comdelegate.h | Changes QCall signature for Delegate_AdjustTarget. |
| src/coreclr/vm/comdelegate.cpp | Adds preemptive transitions around method desc association/entrypoint resolution; updates AdjustTarget QCall. |
| src/coreclr/vm/comconnectionpoints.cpp | Caches entrypoints in preemptive mode before invoking managed entrypoints. |
| src/coreclr/vm/comcallablewrapper.cpp | Changes a contract to MODE_PREEMPTIVE. |
| src/coreclr/vm/codeman.h | Adds a stub ReportStubBlock with preemptive-mode contract when perfmap is off. |
| src/coreclr/vm/codeman.cpp | Unguards PerfMap logging for jump stub emission. |
| src/coreclr/vm/class.cpp | Updates boxing/unboxing entrypoint helpers to MODE_PREEMPTIVE. |
| src/coreclr/vm/ceemain.cpp | Unguards PerfMap header include. |
| src/coreclr/vm/callhelpers.h | Refactors MethodDescCallSite to take an explicit PCODE call target. |
| src/coreclr/vm/callhelpers.cpp | Adjusts default ctor invocation to precompute callable entrypoint in preemptive region. |
| src/coreclr/vm/assembly.cpp | Wraps GetSingleCallableAddrOfCode in preemptive region for main entry. |
| src/coreclr/vm/arm64/stubs.cpp | Unguards dynamic helper PerfMap logging macro. |
| src/coreclr/vm/arm/stubs.cpp | Unguards dynamic helper PerfMap logging macro. |
| src/coreclr/vm/amd64/cgenamd64.cpp | Unguards dynamic helper PerfMap logging macro; modifies END macro behavior. |
| src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs | Updates AdjustTarget interop to pass MethodTable* and keep target alive. |
| src/coreclr/debug/ee/funceval.cpp | Adds preemptive transitions around call target resolution for func-eval. |
|
Switching more C/C++ loader code that does heavy lifting to preemptive mode is general goodness. The test failures are related and needs fixing: |
|
@jkotas This should be good overall, but I don't think it's appropriate for the servicing fix we need to take, but given that its basically functional, I plan to take #129021 for the fix to the PerfMap bug in .NET 10, (and also in the main branch), and then follow it up with this change which effectively removes all of the potential negative side effects of #129021. I was quite surprised by how small the delta turned out to be for this change. |
…runtime into CallPerfMapFromPREEMPTIVE
… issues around reading EnC fields to use volatile load/store operations as there is no apparent synchronization.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 71 out of 71 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
src/coreclr/vm/stublink.cpp:564
- PerfMap::LogStubs now requires MODE_PREEMPTIVE (see perfmap.cpp contract), but this call site doesn't enforce preemptive mode. In contract-enabled builds this risks a contract assert if Link() is entered while cooperative. Consider switching to preemptive just for the logging call (and optionally guarding by PerfMap::IsEnabled() to avoid unnecessary mode transitions when disabled).
src/coreclr/vm/threads.cpp:1047 - PerfMap::LogStubs requires MODE_PREEMPTIVE, but this call is currently made without an explicit preemptive transition. If InitThreadManagerPerfMapData runs in cooperative mode (common during initialization paths), this can trip contracts. Switching to preemptive just for the log call keeps the new PerfMap contracts honest.
src/coreclr/vm/prestub.cpp:374 - PerfMap::LogPreCompiledMethod is now reachable even when FEATURE_PERFMAP is not defined, and it requires MODE_PREEMPTIVE. This call site doesn't explicitly switch to preemptive, which can cause contract asserts if PrepareCode runs in cooperative mode. Consider switching to preemptive just around the log call (and optionally guard by PerfMap::IsEnabled()).
src/coreclr/vm/prestub.cpp:926 - PerfMap::LogJITCompiledMethod requires MODE_PREEMPTIVE, but this call site doesn't explicitly ensure preemptive mode. Switching to preemptive just for the log call keeps PerfMap contracts valid and avoids contract asserts when this code runs in cooperative mode.
src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs:981 - IRuntimeMethodInfo.GetValue now depends on a layout contract ("m_value at the same offset" across all implementers) and uses Unsafe.As(method) to read it. This is brittle and can silently break if field layouts change (e.g., new fields, different field packing rules, different runtimes). Consider reverting to an explicit interface member (as before) or otherwise adding a strong, debug-time validation of the assumed layout/offsets so layout drift fails fast.
internal interface IRuntimeMethodInfo
{
internal static RuntimeMethodHandleInternal GetValue(IRuntimeMethodInfo method)
{
// All implementations of IRuntimeMethodInfo are required to have a m_value field at the same offset as RuntimeMethodInfoStub.m_value.
return new RuntimeMethodHandleInternal(Unsafe.As<RuntimeMethodInfoStub>(method).m_value);
}
Update generate-coreclr-helpers.cmd/.sh to also generate the wasi callhelper sources (in addition to browser) by running the generator with TargetOS=wasi against the wasi testhost, emitting to src/coreclr/vm/wasm/wasi/. Add a --wasi-scan-path override and a markdown doc describing the prerequisite builds. Fix a latent cmd bug where repo_root was computed after SHIFT clobbered %0. Regenerate the wasi callhelper sources. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b67a62f2-460e-48e8-9de4-d57f19b3fb16
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 74 out of 74 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
src/coreclr/vm/comdelegate.cpp:1177
- BindToMethodDetails is an out-parameter for a QCall, but this method doesn’t initialize all fields on every path. In particular, the closed-delegate path never assigns methodPtrAux (and extraData/loaderAllocatorGCHandle are set later), which can leave managed code reading uninitialized data from the out struct. Initialize the struct fields up front (at least methodPtrAux) so every field has a deterministic value regardless of which branch is taken.
pTargetMethod->EnsureActive();
if (fIsOpenDelegate)
src/coreclr/vm/comdelegate.cpp:1675
- Delegate_Construct returns BindToMethodDetails via an out struct, but the closed-delegate branch never assigns methodPtrAux. Managed code always copies methodPtrAux into the delegate instance, so this can propagate an uninitialized pointer. Initialize methodPtrAux (and other fields) before branching so the out struct is fully defined for both open and closed delegates.
// If the loader allocator is not collectible, this will be NULL
pBindToMethodDetails->loaderAllocatorGCHandle = pMeth->GetLoaderAllocator()->GetLoaderAllocatorObjectHandle();
// Open delegates.
if (invokeArgCount == methodArgCount)
src/coreclr/vm/stublink.cpp:564
- PerfMap::LogStubs now has a MODE_PREEMPTIVE contract, but StubLinker::Link doesn’t establish preemptive mode (STANDARD_VM_CONTRACT only declares THROWS). This risks contract violations in debug builds and undermines the goal of keeping PerfMap interactions preemptive-only. Wrap the log call in a preemptive-mode scope (or update the function contract accordingly).
The baked framework P/Invoke table (libcoreclr_gen_static.a) now references wasi:http imports pulled in by System.Net.*. Those aren't part of the wasi:cli/command world the bundled adapter provides, so declare them to the component encoder via --component-type, matching WasiApp.CoreCLR.targets. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c9cd42b-98d5-4fed-b0fb-c3122b890259
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs:981
- IRuntimeMethodInfo.GetValue relies on Unsafe.As(method) and a presumed field-offset/layout agreement across all implementors. That assumption is not enforced by the type system and can silently break if any IRuntimeMethodInfo implementor changes its field layout (or if a new implementor is added), leading to incorrect handles and hard-to-debug failures.
Consider restoring a real interface member (e.g., RuntimeMethodHandleInternal Value { get; }) with explicit implementations, or otherwise providing a type-safe mechanism to extract the handle without depending on object layout.
internal interface IRuntimeMethodInfo
{
internal static RuntimeMethodHandleInternal GetValue(IRuntimeMethodInfo method)
{
// All implementations of IRuntimeMethodInfo are required to have a m_value field at the same offset as RuntimeMethodInfoStub.m_value.
return new RuntimeMethodHandleInternal(Unsafe.As<RuntimeMethodInfoStub>(method).m_value);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/coreclr/vm/comdelegate.cpp:1178
- BindToMethodDetails is an out struct returned to managed code, but COMDelegate::BindToMethod does not initialize all fields on the closed-delegate path (e.g., methodPtrAux is never assigned). This can propagate uninitialized stack garbage into Delegate._methodPtrAux when the QCall succeeds, causing unpredictable crashes. Consider zero-initializing the struct at the start of the method (and ideally add a PRECONDITION for pBindToMethodDetails).
src/coreclr/vm/comdelegate.cpp:1635 - Delegate_Construct returns BindToMethodDetails via an out parameter, but the closed-delegate path never assigns methodPtrAux. Since managed code always copies bindToMethodDetails.methodPtrAux into Delegate._methodPtrAux on success, leaving this field uninitialized can corrupt the delegate instance. Zero-initialize *pBindToMethodDetails early in the QCall so all fields have defined defaults.
corerun now imports wasi:http (baked into libcoreclr_gen_static.a via System.Net.*), so the wasmtime host must provide it or the component fails to instantiate. Add -S http to the runtime-test launch, matching the library test path (tests.wasi.targets) and the corerun component-type link flag. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6c9cd42b-98d5-4fed-b0fb-c3122b890259
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 76 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/coreclr/vm/stublink.cpp:565
- PerfMap::LogStubs now has a MODE_PREEMPTIVE contract, but StubLinker::Link is under STANDARD_VM_CONTRACT and doesn't ensure preemptive mode before calling it. This will trip contracts in checked builds if Link is entered in cooperative mode (and it’s also inconsistent with the PR goal of only calling PerfMap in preemptive). Wrap the LogStubs call in a GCX_PREEMP() scope.
src/coreclr/vm/amd64/cgenamd64.cpp:612 - BEGIN_DYNAMIC_HELPER_EMIT calls PerfMap::LogStubs, which now requires MODE_PREEMPTIVE. The macro expansion doesn't ensure preemptive mode, so any cooperative callers will violate contracts. Consider wrapping the LogStubs call in a GCX_PREEMP() scope inside the macro (and apply the same pattern to the other architecture helper emit macros).
#define BEGIN_DYNAMIC_HELPER_EMIT(size) \
BEGIN_DYNAMIC_HELPER_EMIT_WORKER(size) \
PerfMap::LogStubs(__FUNCTION__, "DynamicHelper", (PCODE)p, size, PerfMapStubType::Individual);
src/coreclr/vm/i386/cgenx86.cpp:568
- Same issue as amd64: BEGIN_DYNAMIC_HELPER_EMIT calls PerfMap::LogStubs (MODE_PREEMPTIVE) without ensuring preemptive mode first. Wrap the LogStubs call in GCX_PREEMP() within the macro to keep contracts consistent.
…fMapFromPREEMPTIVE
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 76 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/coreclr/vm/comdelegate.cpp:1176
COMDelegate::BindToMethodreturnsBindToMethodDetailsto managed code, but the struct isn’t fully initialized on all paths (notablymethodPtrAuxin the closed-delegate case). Since the managed caller always assigns_methodPtrAuxfrom this struct, any uninitialized field can turn into delegate state corruption.
Suggestion: zero-initialize the out struct at the start of the function, then overwrite fields as needed in the open/closed branches.
src/coreclr/vm/comdelegate.cpp:1673
Delegate_ConstructpopulatesBindToMethodDetailsfor managed code to apply onto the delegate instance, butmethodPtrAuxisn’t written on the closed-delegate path. Since the managed side always assigns_methodPtrAuxfrom this struct, leaving it uninitialized can corrupt delegate state.
Recommendation: initialize the out struct fields to known defaults before the open/closed delegate branches.
src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs:981
IRuntimeMethodInfo.GetValuerelies onUnsafe.As<RuntimeMethodInfoStub>(method)and a comment-level contract that allIRuntimeMethodInfoimplementations have anIntPtrfield at the same offset asRuntimeMethodInfoStub.m_value. This is a very brittle invariant: it’s not enforced by the type system, and any futureIRuntimeMethodInfoimplementer (or layout change in existing ones) could silently return the wrong handle and cause hard-to-debug runtime corruption.
If the goal is to avoid calling into managed code from preemptive regions, consider using an explicit interface member again (even if it’s just a trivial field return) or otherwise enforce/validate this layout coupling in one place rather than depending on Unsafe.As across unrelated types.
internal interface IRuntimeMethodInfo
{
internal static RuntimeMethodHandleInternal GetValue(IRuntimeMethodInfo method)
{
// All implementations of IRuntimeMethodInfo are required to have a m_value field at the same offset as RuntimeMethodInfoStub.m_value.
return new RuntimeMethodHandleInternal(Unsafe.As<RuntimeMethodInfoStub>(method).m_value);
}
This PR makes the PerfMap apis available on Windows, where our contracts work, and provides stub implementations which have the appropriate contracts. Then, I went through the codebase and found the places where we would need to transition to preemptive to make it all safe. As I found a number of the codepaths were actually GC_NOTRIGGER code paths, I pushed the work upstream until the logic primarily converted was all calls to entrypoint apis, and all non-allow creation calls to
FindOrCreateAssociatedMethodDesc